-
Notifications
You must be signed in to change notification settings - Fork 273
Extend Functionality to Use the Presolver Plugin and Add a Tutorial #1076
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
…s in the knapsack model
…for fixed and aggregated variables
…, and example shiftbound.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1076 +/- ##
==========================================
+ Coverage 54.13% 54.70% +0.56%
==========================================
Files 22 24 +2
Lines 5045 5327 +282
==========================================
+ Hits 2731 2914 +183
- Misses 2314 2413 +99 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR extends PySCIPOpt to support custom presolver plugins by adding wrapper functions, tests, a working example, and documentation. The key enhancement enables users to implement custom presolvers in Python by subclassing the Presol class.
- Added
isActive()method for variables to check if they are active (not fixed or aggregated) - Added
aggregateVars()method to aggregate two variables with custom coefficients - Created a complete Shiftbound presolver example that replicates SCIP's boundshift.c logic
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/pyscipopt/scip.pxi | Added isActive() and aggregateVars() wrapper methods for variables and models |
| src/pyscipopt/scip.pxd | Added C function declarations for SCIPvarIsActive and SCIPaggregateVars |
| tests/test_vars.py | Added basic test for isActive() method on newly created variables |
| tests/test_aggregate_vars.py | Added comprehensive tests for aggregateVars() functionality |
| examples/finished/shiftbound.py | Complete presolver example implementing boundshift logic |
| docs/tutorials/presolver.rst | Tutorial documentation explaining presolver plugin usage |
| CHANGELOG.md | Updated changelog with new features |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
mmghannam
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much @fvz185 for the useful contribution! I believe this will make it much easier for people to prototype presolving techniques in python.
I've added a few small comments otherwise looks good to merge to me :)
| if val > 0 and val >= scip.infinity(): | ||
| return scip.infinity() | ||
| if var.vtype() != "CONTINUOUS": | ||
| return scip.feasCeil(val) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be wrong, but shouldn't this be rounding to the nearest integer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right. It's actually only adjustedLB() and forgot to include adjustedUB(). I guess that is a good example for why we want to wrap functions instead of copying their functionality.
| for var in reversed(vars): | ||
| if var.vtype() == "BINARY": | ||
| continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be an assert? I assume all binary variables are skipped from earlier.
| ) | ||
| # Aggregate old variable with new variable: | ||
| # x = y + lb (no flip), or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about writing this as x - y = lb to be the same as what is passed to SCIPaggregateVars?
| assert not infeasible | ||
| assert aggregated | ||
| # model should stay consistent | ||
| model.optimize() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add assertion to check the bounds of the variables? also an assertion for the final status and the objective value of the model would be nice :)
|
|
||
| def aggregateVars(self, Variable varx, Variable vary, scalarx=1.0, scalary=-1.0, rhs=0.0): | ||
| """ | ||
| Aggregate varx and vary: calls SCIPaggregateVars and returns |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add more info on what SCIPaggregateVars does? a formula would be very helpful here so that one wouldn't need to go the C-API documentation.
|
To also answer your questions:
I'm happy with the current test, but if you have the time yes, assertions in your presolver test on the aggregated variables would be nice.
I don't have a lot of experience with presolvers but I would say one should wrap the methods when a use-case/feature request pops up. If you see anything that could be useful to "preemptively" wrap I'd be happy to review it in future PRs :D |
|
I just realised that, while I was working on the documentation, the implementation of presol_boundshift.c had changed. Besides slight modifications to the sequence, it now no longer uses SCIPvarIsActive(). I'll check and align my documentation again but prioritise to make the presolver documentation/example self-contained/-explanatory. I'll also take a look again on the tests to capture your suggestions. Lastly, I think I will crawl through the other presolvers to see which SCIP functions are most frequently used and then wrap them (as part of future PRs). My goal is anyway to better grasp SCIP's source code, so it'll be a good exercise. Just let me know about any other feature that I could take a look at (ofc, I'll also take a look at the currently open issues). Thank you @mmghannam for your feedback :) |
|
Hey @fvz185 ! THank you so much for your contribution! We would love to include this in the next release, PySCIPOpt 7.0.0, which coincides with SCIP 10 (should be a couple more weeks, end of the month, start of next month). How are the TODOs looking, is there any way we can help out? |
|
|
||
| Here is a high-level flow: | ||
|
|
||
| 1. Subclass ``MyPresolver`` and capture any parameters in ``__init__``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 1. Subclass ``MyPresolver`` and capture any parameters in ``__init__``. | |
| 1. Create subclass ``MyPresolver`` and capture any parameters in ``__init__``. |
(unless Subclass is a verb)
| presolver that shifts variable bounds from ``[a, b]`` to ``[0, b - a]`` | ||
| and optionally flips signs to reduce constant offsets. | ||
|
|
||
| For educational purposes, we keep our example as close as possible to SCIP's implementation, which can be found `here <https://scipopt.org/doc-5.0.1/html/presol__boundshift_8c_source.php>`__. However, one may implement Boundshift differently as SCIP's logic does not translate perfectly to Python. To avoid any confusion with the already implemented version of Boundshift, we will call our custom presolver *Shiftbound*. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| For educational purposes, we keep our example as close as possible to SCIP's implementation, which can be found `here <https://scipopt.org/doc-5.0.1/html/presol__boundshift_8c_source.php>`__. However, one may implement Boundshift differently as SCIP's logic does not translate perfectly to Python. To avoid any confusion with the already implemented version of Boundshift, we will call our custom presolver *Shiftbound*. | |
| For educational purposes, we keep our example as close as possible to SCIP's implementation, which can be found `here <https://scipopt.org/doc-5.0.1/html/presol__boundshift_8c_source.php>`__. However, one may implement Boundshift differently, as SCIP's logic does not translate perfectly to Python. To avoid any confusion with the already implemented version of Boundshift, we will call our custom presolver *Shiftbound*. |
| Presolvers | ||
| ########### | ||
|
|
||
| For the following let us assume that a Model object is available, which is created as follows: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| For the following let us assume that a Model object is available, which is created as follows: | |
| For the following, let us assume that a Model object is available, which is created as follows: |
|
|
||
| A complete working example can be found in the directory: | ||
|
|
||
| - ``examples/finished/shiftbound.py`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe shiftbound_presolver.py or presol_shiftbound.py or some variation, to make it easier for users later on?
| def presolexec(self, nrounds, presoltiming): | ||
| scip = self.model | ||
| # Utility replacements for a few SCIP helpers which are not exposed to PySCIPOpt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please interface these methods instead?
I can do it later as well, no problem
To clarify and document the usage of PySCIPOpt's presolver plugin, a working example and a tutorial were added. They required wrappers to be implemented, too. The wrapper functions are supplemented with tests. The example and tutorial replicate the Boundshift presolver.
However, some open issues remain: